Display read receipts next to each message as a row of Avatars for users who have seen that message #162
Conversation
|
[WIP] Will try to change sequencer to follow GenUI's way of creating a list of widgets |
|
Using the existing Alternatively, we can create a separate function (or further modify |
Co-authored-by: Kevin Boos <[email protected]>
Co-authored-by: Kevin Boos <[email protected]>
Co-authored-by: Kevin Boos <[email protected]>
…brix into read_receipt_display
kevinaboos
added
waiting-on-review
There was a problem hiding this comment.
Thanks for refactoring the code; it does look nicer now.
I'm not sure how thoroughly you tested this, but I experience quite a few bugs or odd behaviors on my machine when testing it.
First, the on-hover tooltip is frequently cut off, so let's reposition that differently.
Second, the tooltip also doesn't disappear properly when you hover out of the avatar row:
Third, the log is absolutely filled with these messages:
src/sliding_sync.rs:433:21 - Sending get user profile request: ...
which indicates that you're not correctly using the user profile cache, as I see tons of duplicate requests. If you use the cache correctly (and do not re-draw the AvatarRow until a cache update is received), then you should see only one request max per unknown user ID.
Finally, and most importantly, the displayed read receipt info is actually incorrect and/or incomplete. If you scroll through any larger public room (e.g., Element Web/Desktop, Matrix HQ, etc) you'll see a total mismatch between the read receipts displayed on Robrix and the read receipts displayed by Element in the same room. Not sure what you're doing to cause this, but please look into the correctness/accuracy of the read receipts. Perhaps they're not getting updated?
Read receipts also seem to be completely missing for quite a few messages: I see them in Element but not in Robrix. Please investigate; this is probably related to the above issue too.
Finally, address all the compiler warnings that your PR has added (the main branch currently has 5 warnings, which you can ignore).
src/home/room_read_receipt.rs
Outdated
| pub fn set_avatar_row<'a, T>(&mut self, cx: &mut Cx, room_id: &RoomId, event_id: Option<&EventId>, receipts_len: usize, receipts_iter: T) | ||
| where T:Iterator<Item = (&'a OwnedUserId, &'a Receipt)> { |
There was a problem hiding this comment.
this function can be simplified in two ways.
- There's no need for generics here. Just directly accept the IndexMap of read receipts, which will also allow you do to number 2 below.
- Remove the
receipts_lenparameter, since that is completely unnecessary and error-prone, as it's separate from (and redundant with) theIndexMapof read receipts.
src/home/room_read_receipt.rs
Outdated
| impl Widget for AvatarRow { | ||
| fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) { | ||
| let uid = self.widget_uid(); | ||
| for button in self.buttons.iter_mut() { |
There was a problem hiding this comment.
isn't this a very expensive way to check for a hit? Surely we should just be checking the full area of the AvatarRow as a single "whole" entity, instead of checking each button.
Unless, that is, you're going to allow each mini avatar in the AvatarRow to be individually-clickable ... which is fine, but I don't think you're doing that.
kevinaboos
added
waiting-on-author
kevinaboos
changed the title
…read_receipt_display
…read_receipt_display
Co-authored-by: Kevin Boos <[email protected]>
…brix into read_receipt_display
alanpoon
added
waiting-on-review
alanpoon
added
waiting-on-author
Fixes #153